Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Blocked] feat: adding more fields to vault scalar #77

Closed
wants to merge 28 commits into from
Closed

[Blocked] feat: adding more fields to vault scalar #77

wants to merge 28 commits into from

Conversation

jtfirek
Copy link
Contributor

@jtfirek jtfirek commented Feb 1, 2023 •
edited
Loading

Pr for adding to vault scalar.

@jtfirek jtfirek requested a review from peterslany February 1, 2023 21:10
bvotteler
bvotteler reviewed Feb 2, 2023
View reviewed changes
runProcessor.sh Outdated Show resolved Hide resolved
bvotteler
bvotteler requested changes Feb 2, 2023
View reviewed changes
src/mappings/event/vault.ts Outdated Show resolved Hide resolved
src/mappings/event/vault.ts Outdated Show resolved Hide resolved
src/mappings/event/vault.ts Outdated Show resolved Hide resolved
src/model/generated/vault.model.ts Outdated Show resolved Hide resolved
@jtfirek jtfirek marked this pull request as draft February 2, 2023 20:31
@jtfirek jtfirek changed the title feat: adding CollateralAmount to vault feat: adding more fields to vault scalar Feb 2, 2023
@jtfirek jtfirek mentioned this pull request Feb 2, 2023
Merged
@jtfirek
Copy link
Contributor Author

jtfirek commented Mar 9, 2023 •
edited
Loading

Do you guys have any thoughts on the assumptions I have made so far in this pr @nud3l @bvotteler :

  • New vaults without collateral have their percent collateral set to -1.
  • New empty vaults are set to secure by default.
  • Pulling the threshold values from storage is very expensive so right now thresholds are only updated when a change has been made to the vault collateral or Btc amount.
  • The event I added to the parachain for Active/Disabled will not be merged for a few weeks so I left this value null for now in the vault scalar.

@jtfirek jtfirek marked this pull request as ready for review March 9, 2023 17:22
bvotteler
bvotteler requested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@bvotteler bvotteler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of smaller issues to get sorted, a few of those just formatting related.

And two other important bits:

  • Please restore the .env file to what it was
  • You need to merge master in to resolve conflicts.

I suggest to regenerate the database migration files from scratch after merging master back in. Feel free to simply remove all migration files and regenerate a single one.
We will need to reindex from the start as we deploy the next version anyway.

src/mappings/event/issue.ts Outdated Show resolved Hide resolved
src/mappings/event/issue.ts Outdated Show resolved Hide resolved
src/mappings/event/issue.ts Outdated Show resolved Hide resolved
src/mappings/event/issue.ts Outdated Show resolved Hide resolved
src/mappings/event/oracle.ts Show resolved Hide resolved
src/mappings/event/vault.ts Outdated Show resolved Hide resolved
src/mappings/utils/updateVault.ts Outdated Show resolved Hide resolved
src/mappings/utils/updateVault.ts Outdated Show resolved Hide resolved
src/mappings/utils/vaultThresholds.ts Outdated Show resolved Hide resolved
src/mappings/utils/vaultThresholds.ts Outdated Show resolved Hide resolved
@bvotteler
Copy link
Contributor

  • Pulling the threshold values from storage is very expensive so right now thresholds are only updated when a change has been made to the vault collateral or Btc amount.

We could also check in with the parachain devs whether we can get an event added for thresholds being set. That would save us from having to query state.

@jtfirek jtfirek mentioned this pull request Mar 13, 2023
Closed
@jtfirek jtfirek marked this pull request as draft March 13, 2023 23:24
bvotteler
bvotteler reviewed Mar 15, 2023
View reviewed changes
Copy link
Contributor

@bvotteler bvotteler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as comment only (instead of change requested), so I won't block approvals while I'm out.

.env
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore this file so it doesn't mess with README instructions for quick starts.

@jtfirek jtfirek changed the title feat: adding more fields to vault scalar [Blocked] feat: adding more fields to vault scalar Mar 21, 2023
@jtfirek
Copy link
Contributor Author

jtfirek commented Mar 21, 2023

Once these events are merged into the blockchain, I will be able complete this pr.
interlay/interbtc#961
interlay/interbtc#967

@bvotteler
Copy link
Contributor

Closing stale PR

@bvotteler bvotteler closed this Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvotteler bvotteler bvotteler requested changes

@peterslany peterslany Awaiting requested review from peterslany

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtfirek @bvotteler